[webview_flutter] Implement clearLocalStorage/onHttpError and add integration tests based on upstream v4.13.1 - #1069
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements clearLocalStorage and onHttpError for the Tizen WebView, and addresses races, use-after-frees, and crashes during WebView disposal. Additionally, integration tests are updated to await controller calls instead of using unawaited. The reviewer feedback suggests awaiting the controller calls in the newly added onHttpError tests to maintain consistency with these changes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
JSUYA
left a comment
There was a problem hiding this comment.
Do the parts related to dispose and evas_object_del ensure that it works without issues even when displaying multiple views?
| // Set under mutex_ at the start of Dispose(). The raster thread checks it | ||
| // in ObtainGpuSurface() and stops being handed engine-owned TBM surfaces | ||
| // that are about to be freed by the deferred evas_object_del(). | ||
| bool is_disposing_ = false; | ||
| // Set to false at the start of Dispose(). A pending "navigationRequest" | ||
| // reply from Dart (resolved asynchronously) captures a copy and checks it | ||
| // before dereferencing this WebView, avoiding a use-after-free. | ||
| std::shared_ptr<bool> is_alive_ = std::make_shared<bool>(true); |
There was a problem hiding this comment.
Please delete all unnecessary comments.
If you wish to explain how disposing and is_alive work, please leave an explanation in the PR.
Please only leave comments for parts that must be referenced for functionality (TODO, NOTE) or descriptions that need to be published.
| }; | ||
| auto* context = new TeardownContext{instance, std::move(pool)}; | ||
| texture_registrar_->UnregisterTexture(GetTextureId(), [context]() { | ||
| ecore_main_loop_thread_safe_call_async( |
There was a problem hiding this comment.
The ecore API is no longer in use.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ee5d4624f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
1c44536 to
bd27a24
Compare
While the WebView can functionally support multiple views, this isn't currently supported in practice due to memory-leak and crash issues. I will create issues regarding these problems. |
Add Tizen native implementations for two previously unimplemented APIs: - clearLocalStorage: clears web local storage via ewk_context_web_storage_delete_all. - onHttpError: reports HTTP error status codes (>= 400) to the navigation delegate via the policy,response,decide callback. Bump version to 0.10.1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Port the remaining runnable upstream test cases from webview_flutter v4.13.1: - NavigationDelegate > onHttpError - NavigationDelegate > onHttpError is not called when no HTTP error is received - clearLocalStorage These pass thanks to the new clearLocalStorage and onHttpError implementations. The other upstream test cases remain omitted because they cannot run on Tizen: window.open/new-window behavior, HTTP basic auth, and media playback policy are not supported by the engine. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The test file wrapped most controller setup calls (setJavaScriptMode, setNavigationDelegate, loadRequest, etc.) in unawaited(), a leftover from an older upstream version. Match the upstream v4.13.1 style by awaiting those calls instead, keeping unawaited() only where upstream does (the request server loop and the two onHttpError tests). No behavior change; the full suite still passes on the device. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
getScrollPosition() settles asynchronously after scrollTo/scrollBy, so reading it once right after the call was flaky (more so on software-GL rendering such as emulators). Poll the scroll position until it reaches the expected value, with a timeout, so the test waits for the value to settle instead of failing on a transient stale read. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rash Rework WebView::Dispose() to tear resources down safely: - Detach all engine callbacks (including the missing "policy,response,decide") and defer evas_object_del() until the embedder's UnregisterTexture completion callback, so the engine-owned TBM surfaces are not freed while a raster-thread frame is still reading them (flutter-tizen/embedder#182). - Add is_alive_/is_disposing_ guards so async callbacks arriving after disposal no longer touch the destroyed WebView. - On the Tizen 10.0 TV emulator (TV_PROFILE + x86_64), hide the stopped view instead of deleting it to avoid a SIGSEGV in chromium-efl's ~SelectionControllerEfl(); revert once the engine fix ships. Verified on the TV 10.0 emulator: example integration tests previously crashed on WebView disposal and now pass 19/19.
…r-pool UAF - Fix a use-after-free in BufferPool: the engine's release_callback for an in-flight frame can fire on the raster thread after the owning BufferUnit has already been destroyed on the platform thread, dereferencing freed memory. Track live BufferUnits in a mutex-guarded registry and have the callback check it before touching the buffer. tbm_pool_ is now a shared_ptr so its lifetime extends through the deferred teardown below. - Replace ecore_main_loop_thread_safe_call_async() with GLib, following the Ecore removal in flutter-tizen#1033 / flutter-tizen#1045 / flutter-tizen#1046. Must be g_timeout_add_full() at G_PRIORITY_HIGH, not g_idle_add(): an idle source runs too late and lets the delete race the raster thread. - Narrow the TV_PROFILE compile-time macro to a runtime getenv("ELM_PROFILE") check for the same evas_object_hide()-instead-of-del() workaround. Still needed: even with the buffer-pool fix above, evas_object_del() can intermittently crash the raster thread on the Tizen 10.0 TV emulator, and this replaces the compile-time check the review flagged. - Trim the disposal comments down to the constraints; the ordering rationale moves to the PR description. Verified via flutter-tizen drive: - TV 10.0 x86_64 emulator: 8/8 consecutive runs green (0 crashes). - Real TV device (armv7l): one full clean run (19/19); further repeats hit app-launch failures unrelated to this change. I will create a new issue for this situation.
…ple WebViews
chromium-efl's ewk_shutdown() fatally CHECKs if any Ewk_View is still
alive ("Client didn't destroy all WebView objects before calling
ewk_shutdown"). ewk_init()/ewk_shutdown() are now called once for the
process lifetime from WebviewFlutterTizenPlugin's constructor/destructor,
but WebView::Dispose() only queues its evas_object_del() behind an
UnregisterTexture() callback and a g_timeout_add_full() hop onto the
main loop, so a view could still be alive when the plugin destructor
runs ewk_shutdown().
Add a PendingTeardown registry that tracks each deferred
evas_object_del() from Dispose(), and WebView::FlushPendingTeardowns(),
called from the plugin destructor right before ewk_shutdown(), which
pumps the GLib main loop until the registry drains (or force-deletes
any stragglers past a 2s deadline, since a leaked view is a guaranteed
fatal CHECK).
This also removes the TV emulator's evas_object_hide() workaround,
which leaked the ewk_view and directly conflicted with ewk_shutdown()'s
contract. That workaround no longer reproduces the raster-thread crash
it was added for, and leaking now causes the exact crash this commit
fixes. Removing it also resolves the "only one WebView at a time"
limitation, since two disposed-but-leaked views could never both be
torn down cleanly.
Verified on the real TV target (10.113.112.246:26101) across 3
consecutive `flutter-tizen drive` runs of the full integration_test
suite: all pass, no SIGSEGV/SIGTRAP, and no new crash report.
…eously Mounts and disposes two WebViews at the same time, which used to hit a native disposal race fixed in the previous commit (see the now-removed "Using more than one WebView at the same time" note in README.md). Verified on the real TV target (10.113.112.246:26101) across 3 consecutive full-suite runs, all passing.
InitWebView() created a new Ecore_Evas per WebView instance and never freed it, leaking one canvas per WebView for the process lifetime. Following DALi's WebEngineManager pattern, WebView instances now share a single canvas created lazily and freed once, after all WebViews are torn down and before ewk_shutdown(). Verified on real TV (10.113.112.246, 8/8 consecutive runs, no crashes) and on the TV emulator (emulator-26111, 10/12, matching the pre-existing flutter-tizen#1077 VizCompositorTh crash rate — no regression from this change).
552c399 to
916cea3
Compare
The "multiple WebViews can be used simultaneously" test's comment cited a "Using more than one WebView at the same time" note in README.md that never existed in the file's history. Point to the actual source instead: CHANGELOG.md's 0.10.1 entry, which documents the disposal-race fix.
The deferred-delete mechanism (raster thread may still read TBM surfaces; ewk_shutdown() fatally CHECKs on a live Ewk_View) was re-explained with slightly different wording at seven separate spots across webview.cc, webview.h, and webview_flutter_tizen_plugin.cc. Consolidate the explanation into a single comment on PendingTeardown and trim the rest to short, non-redundant notes. Also fix CHANGELOG.md's 0.10.1 entry, which described the removed TV emulator workaround backwards: the workaround used evas_object_hide() to avoid calling evas_object_del(), it was not itself an "evas_object_del() workaround".
…GELOG.md The evas_object_hide() workaround was added and removed within this same unreleased 0.10.1 section, so it never shipped. Mentioning its removal in the changelog only confuses readers who never saw it introduced; keep the bullet focused on the fix that actually ships.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97ff977fef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
g_main_context_iteration(ctx, TRUE) blocks until some GLib source becomes ready, with no bound of its own. If a pending teardown's render-thread hop never arrives (e.g. the render thread has already stalled or exited), and no other source is active on the default context, this call can block indefinitely, so the 2-second deadline check on the next loop iteration is never reached. Switch to a non-blocking iteration with a short sleep so the deadline is always re-checked on a bounded interval, regardless of whether the render thread ever responds.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
JSUYA
left a comment
There was a problem hiding this comment.
There is too much excessive implementation. Please reduce duplicate implementations and delete all unnecessary comments.
| return g_shared_canvas; | ||
| } | ||
|
|
||
| // static |
There was a problem hiding this comment.
Please delete all unnecessary comments.
| * Implement `onHttpError` for the navigation delegate. | ||
| * Fix races and use-after-frees on WebView disposal, including a buffer-pool | ||
| use-after-free on the raster thread. | ||
| * Replace the Ecore main loop API with GLib. |
There was a problem hiding this comment.
Is "Replace" correct? There is no that previously used the ecore main loop.
|
|
||
| static Ecore_Evas* GetSharedCanvas(); | ||
|
|
||
| // Must be called after FlushPendingTeardowns() and before ewk_shutdown(). |
There was a problem hiding this comment.
Please delete all unnecessary comments.
| } | ||
|
|
||
| WebviewFlutterTizenPlugin() {} | ||
| WebviewFlutterTizenPlugin() { ewk_init(); } |
There was a problem hiding this comment.
It is not appropriate to call the ewk_ API directly from webview_flutter_tizen_plugin.cc.
| // See WebView::FlushPendingTeardowns() for why this must run first. | ||
| WebView::FlushPendingTeardowns(); | ||
| WebView::FreeSharedCanvas(); | ||
| ewk_shutdown(); |
There was a problem hiding this comment.
It is not appropriate to call the ewk_ API directly from webview_flutter_tizen_plugin.cc.
|
|
||
| virtual ~WebviewFlutterTizenPlugin() {} | ||
| virtual ~WebviewFlutterTizenPlugin() { | ||
| // See WebView::FlushPendingTeardowns() for why this must run first. |
There was a problem hiding this comment.
Please delete unnecessary comments.
| WebView::FlushPendingTeardowns(); | ||
| WebView::FreeSharedCanvas(); | ||
| ewk_shutdown(); |
There was a problem hiding this comment.
I think it can be handled in a single function within the WebView class.
| std::mutex g_pending_teardown_mutex; | ||
| std::vector<std::shared_ptr<PendingTeardown>> g_pending_teardowns; | ||
|
|
||
| Ecore_Evas* g_shared_canvas = nullptr; |
There was a problem hiding this comment.
Why is it "shared" canvas? and it's not even canvas.
| bool disposed_ = false; | ||
| // Guarded by mutex_. Keeps the raster thread from being handed TBM surfaces | ||
| // during the deferred teardown. | ||
| bool is_disposing_ = false; |
There was a problem hiding this comment.
This PR tracks the "disposing state" in four separate places: disposed_, is_disposing_, is_alive_(shared_ptr), PendingTeardown+TeardownContext, and even the buffer_pool's active_buffers set.
Please minimize unnecessary implementations as much as possible.
- Merge disposed_ and is_disposing_ into a single mutex-guarded flag. - Encapsulate ewk_init()/ewk_shutdown()/FlushPendingTeardowns()/offscreen host teardown behind WebView::InitializeEngine()/ShutdownEngine(), so the plugin no longer calls ewk_* APIs directly. - Rename GetSharedCanvas()/FreeSharedCanvas() to GetOffscreenHost()/ FreeOffscreenHost() to reflect that it's an offscreen host, not a shared rendering canvas. - Merge the PendingTeardown and TeardownContext structs. - Trim comments that restated what the code already says. - Fix CHANGELOG wording: this PR never released the ecore main loop approach, so "Use GLib" rather than "Replace ... with GLib".
clearLocalStorageandonHttpError, and bumpwebview_flutter_tizento 0.10.1.flutter-tizen test(or example integration tests) on TV 10.0 emulator — 19/19 passing, including previously-crashing disposal cases